Fix #2263: per-phase consensus timeout defaults (refine/plan/implement) - #2267

No agent-mode design concerns. This PR is purely orchestrator timeout configuration (per-phase overrides + resolver) with no impact on prompt construction, agent invocation, or output handling.

Review

Reviewed every changed file in the diff and traced the resolver's call sites and the field's readers across the repo. The change is well-scoped: only one direct reader of pipeline.config.consensus_timeout_minutes existed (orchestrator/routes/pipelines.py:10701, previously getattr(..., 30)) and it was migrated to the new resolver. No other code path reads the field, so the type flip from int = 30 → int | None = None is safe inside the orchestrator. The HITL decision text at pipelines.py:9732,9752 uses the local consensus_timeout (seconds), so operators see the resolved per-phase value when the timeout fires.

Tests cover the priority chain (override > legacy global > phase default), the legacy back-compat clause, the leak-prevention (per-phase doesn't bleed into others), and the unknown-phase fallback. The phase_str derivation at pipelines.py:10390 (phase if isinstance(phase, str) else phase.value) yields a member of PipelinePhase — "refine", "plan", "implement", or "pr" — and getattr(config, f"consensus_timeout_minutes_{phase}", None) falls through cleanly for "pr" (and for any future phase) via the unknown-phase branch returning 30. Pydantic's ge=1 validator on each override field guarantees raw_timeout >= 1, so the existing max(raw_timeout, 1) defensive clamp at line 10702 is now strictly redundant but harmless.

No blocking issues found.

Non-blocking suggestions

1. Stale example value in docs/guides/sdlc-pipeline.md:1235. The sample JSON still shows "consensus_timeout_minutes": 30. With the new defaults, setting the global to 30 overrides the calibrated per-phase budgets (so plan would shrink from 60 → 30 and implement from 90 → 30) — exactly the opposite of what someone copy-pasting the example would want. Either drop that line from the example (since null/unset is now the recommended default) or replace it with a per-phase override to demonstrate the new shape.

2. Hard-coded 30 in the unknown-phase fallback (orchestrator/models.py:43). PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN.get(phase, 30) re-encodes the legacy default outside the constant dict. If the dict ever grows a new phase or the floor is recalibrated, this number won't track it. Consider PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN.get(phase, PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN["refine"]) or pulling the floor into a named constant. Minor — the current behavior is documented in the test at test_models.py:460, so it's intentional, just a brittle seam.

3. Public-API schema break (advisory, not blocking). The field's type in PipelineConfig.model_json_schema() flips from {"type": "integer"} to {"anyOf": [{"type": "integer"}, {"type": "null"}]} and the default flips from 30 to null. External clients that POST configs are unaffected (omit-or-null is accepted; passing the int still works), but clients that read a serialized config and assume a non-null int — or generated TypeScript types pinned to the old schema — will see a contract change. Worth a one-liner in release notes; no code change needed.

4. Override-vs-global precedence is correct but worth a doc nudge. The legend in concurrent-execution.md:36-39 and sdlc-pipeline.md:1252-1255 clearly states "wins over the legacy global," but a reader who sets both consensus_timeout_minutes=120 and consensus_timeout_minutes_implement=60 may intuit that the larger value would apply (more time to converge). The current behavior — explicit-phase always wins, even if smaller — is the right call (explicit > implicit), it's just non-obvious. A short worked example in the docs would close the gap.

— Authored by egg

No agent-mode design concerns. The delta since my last review at 38e59ac is a docs merge resolution (folding the brc_consensus_progress_gate_seconds row from #2254 into the per-phase timeout tables and rewriting the Timeout Handling paragraph to describe gate → blocker-eval ordering) plus a file-size allowlist bump. Neither touches prompts, model IDs, agent output structure, or sandbox boundaries.

— Authored by egg

Re-review (delta since 38e59ac)

The substantive code change (resolver + per-phase config fields + read-site swap) was already approved and is unchanged on this commit. Since 38e59ac the PR added two main commits inbound (#2250 file-size lint, #2265 progress-gate doc), one merge commit, and two allowlist baseline bumps. I verified each.

Doc merge resolution — correct

Both docs/guides/concurrent-execution.md (lines 36–41 / 724–731) and docs/guides/sdlc-pipeline.md (lines 1252–1257) cleanly fold the per-phase rows from this PR with the new brc_consensus_progress_gate_seconds row from #2265. The Timeout Handling paragraph at concurrent-execution.md:726–731 correctly orders the runtime flow as "per-phase budget elapses → progress gate → blockers eval," which matches the read site (pipelines.py:10701 resolves the per-phase budget; PeerConsensusTracker.handle_timeout() is the post-gate evaluator). No semantic loss from either side.

Allowlist bumps — two of three match main

scripts/file-size-allowlist.yaml:

• orchestrator/peer_consensus.py → 2003 / 85965. Main has peer_consensus.py at exactly 85965 bytes (gh api .../contents?ref=main). PR doesn't touch this file — bump just absorbs main's drift from #2254 that #2250 missed. ✓
• orchestrator/routes/pipelines.py → 15515 / 677159. Main is at 677112 bytes; PR adds 1 line at pipelines.py:10376 (resolve_consensus_timeout_minutes import) plus the read-site swap, ~47 bytes — post-merge result lands at 677159, matching the new baseline. ✓
• scripts/select_tests.py → 1850 / 73711. Stale by 1495 bytes. The bump was made after #2262 landed on main (which grew the file to 73711) but before #2266 landed (which grew it further to 75206). Current main has select_tests.py at 75206 bytes; the PR is behind_by=2 against main, with #2266 as the second behind-commit. After GitHub auto-merges main into this PR (or after merge into main), the resulting tree will pair main's 75206-byte select_tests.py with this PR's 73711 baseline → make lint will fail with select_tests.py: 75206 bytes exceeds allowlist baseline (73711). Non-blocking because the PR's own tree still passes (PR's select_tests.py is at 63828, well under 73711), but the next rebase against main will need another bump. Worth a follow-up commit before merge to avoid breaking lint on main.

Resolver + read site — unchanged, still correct

I re-confirmed the resolver wiring on this commit: models.py:26-43 (priority chain override > legacy global > phase default), pipelines.py:10376 import, pipelines.py:10701 consumption. Only one direct production reader of config.consensus_timeout_minutes exists outside the resolver (a test setter at test_agent_exits_recorded.py:31), so the type flip from int = 30 → int | None = None remains safe.

Previous non-blocking suggestions

Three of four suggestions from my prior review remain unaddressed (stale "consensus_timeout_minutes": 30 example at sdlc-pipeline.md:1235, hardcoded 30 fallback at models.py:43, public-API schema flip note). Author's prerogative — none are blocking. The example at sdlc-pipeline.md:1235 is now arguably worse than stale: with the new calibrated defaults, copy-pasting "consensus_timeout_minutes": 30 actively regresses plan from 60→30 and implement from 90→30. If the author is taking another commit anyway for the select_tests.py baseline, this would be a cheap addition.

No blocking issues found.

— Authored by egg

No agent-mode design concerns. The delta since my last review at fd466d10 is two merge commits from origin/main (06db62b8, 2ee6c035) — both mechanical conflict resolutions in scripts/file-size-allowlist.yaml to absorb #2274's schema change (per-file baselines dropped) and the post-merge pipelines.py byte count. The substantive code paths I reviewed previously (models.resolve_consensus_timeout_minutes, the pipelines.py:10701 read site, the per-phase config fields) are byte-identical on the new tip.

— Authored by egg

No agent-mode design concerns. The delta since my last review at 2ee6c035 is two mechanical changes:

1. Doc conflict resolution (docs/guides/concurrent-execution.md, docs/guides/sdlc-pipeline.md) folding main's OVERSEER_ALERT terminology from #2264/#2277 into this PR's per-phase timeout prose — additive, no semantic loss.
2. orchestrator/models.py — dropped the "PipelineConfig" forward-reference quote on resolve_consensus_timeout_minutes per ruff UP037 (post-#2297 sweep). Pure typing cleanup; no behaviour change.

The substantive code paths (resolve_consensus_timeout_minutes, the per-phase config fields, the pipelines.py:10894 read site) are unchanged from the previously approved commit. No prompts, model IDs, agent output structure, or sandbox boundaries touched.

— Authored by egg

Re-review (delta since 2ee6c03)

The substantive code (resolver, per-phase fields, read-site swap) was already approved at 38e59ac and is unchanged in functionality. Since 2ee6c035 the PR has added two PR-authored commits plus a merge resolution:

• f692be3 — Addresses two of my prior non-blocking suggestions:
  • docs/guides/sdlc-pipeline.md:1232-1246 — drops the stale "consensus_timeout_minutes": 30 from the JSON example and replaces it with prose + a worked override example. Copy-pasters can no longer accidentally regress plan/implement back to 30. ✓
  • orchestrator/models.py:45-47 — replaces the hardcoded 30 fallback with PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN["refine"], with a doc-comment explaining the choice (smallest calibrated budget = safe upper bound for unknown phases). ✓
  • orchestrator/tests/test_models.py:458-461 — test renamed and asserts against the constant. ✓
• 8a6a241 — Doc-only conflict resolution combining per-phase tables (this PR) with the consensus-timeout-HITL → OVERSEER_ALERT migration (#2264) on the concurrent-execution.md and sdlc-pipeline.md config tables and the Timeout Handling section. I traced both files and the resolved narrative correctly orders runtime flow as "per-phase budget elapses → progress gate → publish OVERSEER_ALERT." No semantic loss from either side.
• 57e9fd8 — Drops the PipelineConfig forward-reference quotes per ruff UP037 (post-#2297 ruff bump). Verified at runtime under py3.14: with PEP 649 lazy annotation evaluation, inspect.signature(resolve_consensus_timeout_minutes) resolves to models.PipelineConfig cleanly even though the function is defined before the class. The annotation is never accessed at function-definition time, so the bare reference works.

#2264 OVERSEER_ALERT integration — verified

pipelines.py:9758 (timeout_minutes = int(consensus_timeout / 60)) and pipelines.py:9802 (metadata["consensus_timeout_minutes"] = timeout_minutes) both consume the resolved per-phase consensus_timeout (from pipelines.py:10897's resolve_consensus_timeout_minutes() call). So the OVERSEER_ALERT body and metadata correctly reflect the resolved per-phase value (e.g., "BRC consensus has not converged after 90 minutes" for default-implement). The two changes compose cleanly.

Tests

I ran the relevant suites locally (system py3.14):

• orchestrator/tests/test_models.py — 76 passed, including all 7 TestResolveConsensusTimeoutMinutes cases.
• test_consensus_polling.py, test_consensus_timeout_recheck.py, test_consensus_complete_with_failures.py, test_pipelines_routes.py — 49 passed.
• test_brc_nack_iteration.py, test_consensus_race_on_exit.py, test_short_flow_contract_reviewer.py, test_agent_exits_recorded.py, test_post_timeout_rebaseline.py — 46 passed.

No regressions from the merge.

Non-blocking observation (one new)

orchestrator/models.py:42: return override triggers mypy's no-any-return warning because getattr(config, ..., None) is typed Any. Not CI-breaking — make lint-python runs mypy on gateway shared sandbox only, not orchestrator, so this is silent today. Verified with a minimal repro: annotating override: int | None = getattr(...) clears it cleanly. Mention only because if orchestrator is ever added to the mypy frontier, this seam will flag.

Previous non-blocking suggestions

Two of four are now resolved (above). The remaining two — public-API JSON-schema flip (advisory release-notes nudge) and a worked override-vs-global precedence example in the docs — are unchanged. The new prose at sdlc-pipeline.md:1240-1246 partially addresses the precedence one with the consensus_timeout_minutes_implement: 120 example, so I'm dropping it. The schema-flip note remains advisory only.

No blocking issues found.

— Authored by egg

No agent-mode design concerns. Delta since last review is a single mypy annotation fix in orchestrator/models.py (override: int | None = getattr(...)) — no impact on agent design.

— Authored by egg

Re-review (delta since 57e9fd8)

Single-line delta: orchestrator/models.py:40 annotates override: int | None = getattr(...). This directly addresses the one non-blocking observation from my prior review (mypy no-any-return).

Verified

• Annotation matches the underlying field types — consensus_timeout_minutes_{refine,plan,implement} are all int | None (models.py:420,428,436), and unknown phases fall through to the getattr default None. int | None is the tight union.
• Function return type -> int still holds: override is only returned inside if override is not None.
• git diff prev-review-tip..pr-2267 --stat confirms exactly one file, one insertion, one deletion. No other behavior touched.
• Resolver semantics, per-phase fields, and the pipelines.py read site are unchanged from the previously approved commit.

No blocking issues. The mypy no-any-return seam is now closed pre-emptively, so when orchestrator joins the mypy frontier this won't flag.

— Authored by egg